Use RustOptimize to set optimize #112756

Rustin170506 · 2023-06-18T06:39:54Z

Use RustOptimize to set optimize.

rustbot · 2023-06-18T06:40:00Z

(rustbot has picked a reviewer for you, use r? to override)

clubby789 · 2023-06-18T13:47:04Z

Instead of StringOrBool, why not make an enum that accepts only true, false and the actual possible values for opt-level (0, 1, 2, 3, s, z), so we can prevent incorrect opt-levels being configured?

Rustin170506 · 2023-06-18T14:16:32Z

Instead of StringOrBool, why not make an enum that accepts only true, false and the actual possible values for opt-level (0, 1, 2, 3, s, z), so we can prevent incorrect opt-levels being configured?

I followed this suggestion. #112678 (comment)

clubby789 · 2023-06-18T14:40:17Z

@Kobzol Any thoughts on the approach here?

Kobzol · 2023-06-18T21:43:13Z

Maybe for options parsing it could stay as StringOrBool and we can just validate supported options during parsing, like this.

But in the parsed config it would be much nicer to have it as an enum. The if let StringOrBool::Bool(true) | StringOrBool::String(_) check is currently duplicated at 4 places, but it's not immediately obvious that you should use this check to recognize a release build. It would be better if there was an enum with methods like is_release(&self) -> bool and get_opt_level(&self) -> Option<String>.

Rustin170506 · 2023-06-19T01:23:46Z

It would be better if there was an enum with methods like is_release(&self) -> bool and get_opt_level(&self) -> Option<String>.

Good suggestion! Thanks!

Kobzol · 2023-06-19T06:55:19Z

It's better now, thanks. I would suggest two more things:

Create an enum and call the methods on it. config.is_release() is a bit more "global", maybe it should be e.g. config.rust_opt.is_release() or something like that.
Perform the validation when creating the enum. When you now run bootstrap, but the get_opt_level method is not called (I'm not sure if it's called always), then you will not know that the provided value might be wrong. And then when you run a different command, suddenly it can fail.

But I'll let others chime in :) These are small details.

clubby789 · 2023-06-26T18:46:16Z

I agree with the above suggestions - I'd also like to ask if you can add a test to validate the new behaviour, but LGTM with these changes applied.

Rustin170506 · 2023-06-27T02:04:15Z

Create an enum and call the methods on it. config.is_release() is a bit more "global", maybe it should be e.g. config.rust_opt.is_release() or something like that.

Perform the validation when creating the enum. When you now run bootstrap, but the get_opt_level method is not called (I'm not sure if it's called always), then you will not know that the provided value might be wrong. And then when you run a different command, suddenly it can fail.

Addressed.

I'd also like to ask if you can add a test to validate the new behaviour

Could you please give me some tips about how to add it? Or which test case I can refer to?

Rustin170506 · 2023-06-27T02:04:21Z

Thanks for your review! 💚 💙 💜 💛 ❤️

Rustin170506 · 2023-06-30T01:16:16Z

@clubby789 Friendly ping~ Could you please take a look? Thanks!

clubby789 · 2023-06-30T14:05:46Z

Sorry for the wait - you can look at https://github.com/rust-lang/rust/blob/master/src/bootstrap/config/tests.rs for some examples of testing the config. The changes themselves look good 🙂

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

Rustin170506 · 2023-07-01T09:01:54Z

Sorry for the wait - you can look at master/src/bootstrap/config/tests.rs for some examples of testing the config. The changes themselves look good 🙂

@clubby789 Added. Thanks! I have a small question: How can I run an individual test from the Bootstrap module? Now I only can use ./x.py test src/bootstrap to run all tests.

Rustin170506 · 2023-07-01T10:16:30Z

It's a bit strange that I can pass the test locally. Is there some special configuration needed? Or is there something wrong with my test itself?

clubby789 · 2023-07-01T10:55:57Z

parse_inner by default will look for a config.toml - you have one locally but the CI does not (or is different), so it has different behaviour. You can use the existing parse function in the test file

src/bootstrap/config/tests.rs

clubby789 · 2023-07-01T10:58:43Z

r=me with tests fixed
@bors delegate+

bors · 2023-07-01T10:58:45Z

✌️ @hi-rustin, you can now approve this pull request!

If @clubby789 told you to "r=me" after making some further change, please make that change, then do @bors r=@clubby789

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

Rustin170506 · 2023-07-02T04:07:54Z

parse_inner by default will look for a config.toml - you have one locally but the CI does not (or is different), so it has different behaviour. You can use the existing parse function in the test file

Got it. Thanks!

Rustin170506 · 2023-07-02T04:45:28Z

@bors r=@clubby789

bors · 2023-07-02T04:45:29Z

📌 Commit 7cab8f7 has been approved by clubby789

It is now in the queue for this repository.

bors · 2023-07-02T04:54:18Z

⌛ Testing commit 7cab8f7 with merge be6e38c...

bors · 2023-07-02T07:21:37Z

☀️ Test successful - checks-actions
Approved by: clubby789
Pushing be6e38c to master...

jyn514 · 2023-07-02T08:17:51Z

src/bootstrap/config/tests.rs

+    assert_eq!(parse("").rust_optimize.is_release(), true);
+    assert_eq!(parse("rust.optimize = false").rust_optimize.is_release(), false);
+    assert_eq!(parse("rust.optimize = true").rust_optimize.is_release(), true);
+    assert_eq!(parse("rust.optimize = \"1\"").rust_optimize.get_opt_level(), Some("1".to_string()));


i'd prefer to require this to be an int instead of a string, so rust.optimize = 1 instead of \"1\". no one is using the new config yet so i think we can break hard here.

What about the "s" and "z"? Do you mean we need to use an IntOrString here?

yes, exactly

I'll fix it later.

rust-timer · 2023-07-02T08:38:32Z

Finished benchmarking commit (be6e38c): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.8%	[0.6%, 0.9%]	6
Regressions ❌ (secondary)	1.1%	[0.4%, 1.4%]	7
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.8%	[0.6%, 0.9%]	6

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.6%	[0.5%, 2.4%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.0%	[-3.4%, -0.6%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.2%	[-3.4%, 2.4%]	5

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-4.5%	[-6.0%, -3.0%]	2
All ❌✅ (primary)	-0.7%	[-0.7%, -0.7%]	1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 661.616s -> 662.143s (0.08%)

lqd · 2023-07-02T09:30:31Z

diesel and wg-grammar noise

@rustbot label: +perf-regression-triaged

rustbot assigned clubby789 Jun 18, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 18, 2023

Rustin170506 marked this pull request as ready for review June 18, 2023 06:57

Rustin170506 force-pushed the rustin-patch-bootstrap branch 2 times, most recently from e0da899 to c382acb Compare June 18, 2023 07:14

Rustin170506 force-pushed the rustin-patch-bootstrap branch from 32e4362 to b6d2cde Compare June 19, 2023 01:22

Rustin170506 force-pushed the rustin-patch-bootstrap branch from b6d2cde to cd62f22 Compare June 27, 2023 02:02

Rustin170506 changed the title ~~Use StringOrBool to set optimize~~ Use RustOptimize to set optimize Jun 27, 2023

Rustin170506 force-pushed the rustin-patch-bootstrap branch from cd62f22 to ec19653 Compare June 27, 2023 02:02

Rustin170506 added 2 commits July 1, 2023 16:03

Use RustOptimize to set optimize

e6e2825

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

Add tests for RustOptimize

878eff1

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

Rustin170506 force-pushed the rustin-patch-bootstrap branch from ec19653 to 878eff1 Compare July 1, 2023 08:59

This comment has been minimized.

Sign in to view

clubby789 reviewed Jul 1, 2023

View reviewed changes

src/bootstrap/config/tests.rs Outdated Show resolved Hide resolved

clubby789 reviewed Jul 1, 2023

View reviewed changes

src/bootstrap/config/tests.rs Outdated Show resolved Hide resolved

clubby789 mentioned this pull request Jul 1, 2023

Don't pass --test-args to python -m unitest #113234

Merged

Fix broken tests

7cab8f7

Signed-off-by: hi-rustin <rustin.liu@gmail.com>

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 2, 2023

bors merged commit be6e38c into rust-lang:master Jul 2, 2023

rustbot added this to the 1.72.0 milestone Jul 2, 2023

bors mentioned this pull request Jul 2, 2023

bootstrap: Include line numbers in debuginfo by default for library/compiler profile #104968

Closed

jyn514 reviewed Jul 2, 2023

View reviewed changes

rustbot added the perf-regression Performance regression. label Jul 2, 2023

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use RustOptimize to set optimize #112756

Use RustOptimize to set optimize #112756

Rustin170506 commented Jun 18, 2023 •

edited

Loading

rustbot commented Jun 18, 2023

clubby789 commented Jun 18, 2023 •

edited

Loading

Rustin170506 commented Jun 18, 2023

clubby789 commented Jun 18, 2023

Kobzol commented Jun 18, 2023

Rustin170506 commented Jun 19, 2023

Kobzol commented Jun 19, 2023

clubby789 commented Jun 26, 2023

Rustin170506 commented Jun 27, 2023

Rustin170506 commented Jun 27, 2023

Rustin170506 commented Jun 30, 2023

clubby789 commented Jun 30, 2023 •

edited

Loading

Rustin170506 commented Jul 1, 2023 •

edited

Loading

This comment has been minimized.

Rustin170506 commented Jul 1, 2023

clubby789 commented Jul 1, 2023 •

edited

Loading

clubby789 commented Jul 1, 2023

bors commented Jul 1, 2023

Rustin170506 commented Jul 2, 2023

Rustin170506 commented Jul 2, 2023

bors commented Jul 2, 2023

bors commented Jul 2, 2023

bors commented Jul 2, 2023

jyn514 Jul 2, 2023 •

edited

Loading

Rustin170506 Jul 2, 2023

jyn514 Jul 2, 2023

Rustin170506 Jul 2, 2023

rust-timer commented Jul 2, 2023

lqd commented Jul 2, 2023

Use RustOptimize to set optimize #112756

Use RustOptimize to set optimize #112756

Conversation

Rustin170506 commented Jun 18, 2023 • edited Loading

rustbot commented Jun 18, 2023

clubby789 commented Jun 18, 2023 • edited Loading

Rustin170506 commented Jun 18, 2023

clubby789 commented Jun 18, 2023

Kobzol commented Jun 18, 2023

Rustin170506 commented Jun 19, 2023

Kobzol commented Jun 19, 2023

clubby789 commented Jun 26, 2023

Rustin170506 commented Jun 27, 2023

Rustin170506 commented Jun 27, 2023

Rustin170506 commented Jun 30, 2023

clubby789 commented Jun 30, 2023 • edited Loading

Rustin170506 commented Jul 1, 2023 • edited Loading

This comment has been minimized.

Rustin170506 commented Jul 1, 2023

clubby789 commented Jul 1, 2023 • edited Loading

clubby789 commented Jul 1, 2023

bors commented Jul 1, 2023

Rustin170506 commented Jul 2, 2023

Rustin170506 commented Jul 2, 2023

bors commented Jul 2, 2023

bors commented Jul 2, 2023

bors commented Jul 2, 2023

jyn514 Jul 2, 2023 • edited Loading

Choose a reason for hiding this comment

Rustin170506 Jul 2, 2023

Choose a reason for hiding this comment

jyn514 Jul 2, 2023

Choose a reason for hiding this comment

Rustin170506 Jul 2, 2023

Choose a reason for hiding this comment

rust-timer commented Jul 2, 2023

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

lqd commented Jul 2, 2023

Rustin170506 commented Jun 18, 2023 •

edited

Loading

clubby789 commented Jun 18, 2023 •

edited

Loading

clubby789 commented Jun 30, 2023 •

edited

Loading

Rustin170506 commented Jul 1, 2023 •

edited

Loading

clubby789 commented Jul 1, 2023 •

edited

Loading

jyn514 Jul 2, 2023 •

edited

Loading